Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add chart-chartjs-financial dependency resolution fix #139

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

walter
Copy link
Contributor

@walter walter commented Oct 11, 2021

Closes #127

Supersedes PR #136

Relies on #138

This fixes the chart-chartjs-financial import by replacing the unreleased as a packagechart-chartjs-financial dependency with temp-chartjs-candlestick which is simply a released version of the same code.

Normally I wouldn't advise using a fork exactly likely this, but currently ember-cli-chartjs is broken without fixing this dependency.

Thanks @devotox for his work on these two PRs and his patience!

devotox and others added 3 commits September 2, 2021 11:14
`chart-chartjs-financial` is unreleased and breaking as a dependency,
swap in `temp-chartjs-candlestick` released package.

Eventually when official release has happened via an npm package, use
that instead.

This fixes the dependency issue and tests pass.
@devotox
Copy link
Contributor

devotox commented Oct 11, 2021

@walter When i try and use this I get an error that says

Uncaught (in promise) Error: "candlestick" is not a registered controller.

@walter
Copy link
Contributor Author

walter commented Oct 11, 2021

@walter When i try and use this I get an error that says

Uncaught (in promise) Error: "candlestick" is not a registered controller.

I see it called in the dummy app application template, but without actually data. So hard to tell if it's functional.

Can you give some code context of how it's being called and some example data, please?

@walter
Copy link
Contributor Author

walter commented Oct 11, 2021

Oh, one other note. When I was testing this add-on from within another app at first it wasn't picking up the changes as it was based on the same repo/branch as previously. I had to blow away yarn.lock and node_modules within that app to get it to pull in changes.

The upshot, if you are testing from an app confirm that the changes have been pulled in by looking in node_modules/ember-cli-chart/package.json and confirm that templ-chartjs-candlestick is in there.

@walter
Copy link
Contributor Author

walter commented Oct 11, 2021

@devotox - never my request for context and code, I managed to recreate error in the test dummy app by passing in datasets in @data. Looking into it.

@devotox
Copy link
Contributor

devotox commented Oct 11, 2021

@walter I was thinking maybe it was different versions of chart.js being used in ember-cli-chart and temp-chartjs-candlestick.

I am pretty sure I used the same versions at 3.5.1 but I may be wrong

One thing I havent tried that crossed my mind maybe using resolutions in package.json

@walter
Copy link
Contributor Author

walter commented Oct 11, 2021

@devotox Yeah, it's weird. It looks like they both are using chart.js 3.5.1, so not a mismatch there. Also it looks like the controller is registered with chart_js.Chart.register(CandlestickController, OhlcController, CandlestickElement, OhlcElement); , but yeah, not resolving.

I wonder if has to do with ember-auto-import resolution. Looking into that now.

@devotox
Copy link
Contributor

devotox commented Oct 12, 2021

@walter could it be that we are importing the index.esm.js file which does not actually register the controllers

Yup just tested it out. What we need to do is

import { CandlestickController, OhlcController, CandlestickElement, OhlcElement } from 'temp-chartjs-candlestick'; // packaged version of chart-chartjs-financial

Chart.register(CandlestickController, OhlcController, CandlestickElement, OhlcElement);

addon/components/ember-chart.js Outdated Show resolved Hide resolved
@devotox
Copy link
Contributor

devotox commented Oct 12, 2021

Also in index.js I think all we need is this

'use strict';

module.exports = {
  name: require('./package').name,
};

Since we are no longer including any files

@walter
Copy link
Contributor Author

walter commented Oct 12, 2021

@devotox thanks for getting that working (I think). I now have the candlestick chart rendering without errors, but I don't have test candlestick or OHLC chart data to verify they work with. Can you try them out locally and let me know if they work for you?

@devotox
Copy link
Contributor

devotox commented Oct 14, 2021

@walter yup tested with ohlc and it works

@walter
Copy link
Contributor Author

walter commented Nov 7, 2021

@walter yup tested with ohlc and it works

I've added a WIP commit that reverts to using chart-charts-financial now that it has a published release. I'm able to see charts in the dummy app, but don't have data to test with. Could you grab the latest and see if your ohlc stuff works with it @devotox?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Chart.js 3.0 support
2 participants